Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use case insentive comparison to get sheet name #3791

Merged
merged 2 commits into from
Nov 18, 2023

Conversation

kpn13
Copy link
Contributor

@kpn13 kpn13 commented Nov 13, 2023

This is:

  • a bugfix
  • a new feature
  • refactoring
  • additional unit tests

Checklist:

  • Changes are covered by unit tests
    • Changes are covered by existing unit tests
    • New unit tests have been added
  • Code style is respected
  • Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
  • CHANGELOG.md contains a short summary of the change and a link to the pull request if applicable
  • Documentation is updated as necessary

Why this change is needed?

For now, the method which check if a sheetname already exists, uses a case-sensitive comparison which can cause issue as Excel doesn't allow to have 2 sheets with the same name and uses a case-insensitive comparison. So, I propose to respect the same rule.

@oleibman
Copy link
Collaborator

Please add expectExceptionMessage to your test (see testNotSerializable in the member you're changing for an example). This helps ensure that the test fails for the "right" reason. I won't insist on it, but it would be helpful if you could do the same for the other tests in this member that call expectException without calling expectExceptionMessage.

@kpn13
Copy link
Contributor Author

kpn13 commented Nov 14, 2023

Thank you for the feedback, it's done!

@oleibman
Copy link
Collaborator

I have found a situation which your change does not cover. I need to think about it a little more.

$spreadsheet = new Spreadsheet();
$sheet1 = $spreadsheet->getActiveSheet();
$sheet1->setTitle('firstsheet');
$sheet2 = $spreadsheet->createSheet();
$sheet2->setTitle('Firstsheet', true, false);

If the third parameter to setTitle is true (default), PhpSpreadsheet will append a number to the sheet name until it finds a unique name. However, if it is false, it won't do that, and so this code will create a corrupt spreadsheet. I do not understand the use case for that third parameter. In the test suite, it is used only in WorksheetTest; I would be surprised if it was ever used "in the wild", and I imagine "caveat emptor" should apply to anyone who uses it. So, I don't think you should do anything about it. But, the first statement in Worksheet::setTitle:

        // Is this a 'rename' or not?
        if ($this->getTitle() == $title) {
            return $this;
        }

For consistency with your change, that should probably be a case-insensitive compare as well. Can you make that change and add an appropriate test in WorksheetTest::testSetTitleDuplicate?

@oleibman
Copy link
Collaborator

No, it turns out that Excel allows you to change the sheet name in this manner (and adjusts defined names etc. accordingly). So the change I suggested to setTitle should not be made after all. Sorry if you wasted any time on it.

@oleibman oleibman merged commit 96fb273 into PHPOffice:master Nov 18, 2023
12 checks passed
@oleibman
Copy link
Collaborator

Thank you for your contribution.

@kpn13
Copy link
Contributor Author

kpn13 commented Nov 19, 2023

Thank you for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants